Prototyping new CodeAction API #36316

mjbvz · 2017-10-16T03:24:10Z

Adds skeleton of a new CodeAction type and allows codeActionProviders to return either Commands or CodeActions

mjbvz · 2017-10-17T00:33:47Z

@jrieken Kai and I were talking today and wanted to try out a different API design than in #34664. The current change introduces two separate interfaces QuickFix and Refactoring instead of a single interface where quick fixes and refactorings are identified by a type field.

The implementation still need lots of work but please take a look at vscode.proposed.d.ts to see what you think of this proposal

jrieken · 2017-10-17T16:18:29Z

I like the general direction but not the change 🙊. I think the Refactoring-type is just a complicated way of expressing the same Command does, a bit of UI (label, tooltip), the actual command id, and some arguments. For the Quickfix I would use some composition, maybe

class QuickFix {
 command: Command
 addresses: Diagnostics[]
}

alternatively, in case we make add support for text edits, add a title

class QuickFix {
  title: string;
  fix: Command | WorkspaceEdit | TextEdit
  addresses: Diagnostics
}

or making it even more compact, with only one type

class CodeAction {
 // ... all from above

 // when set treated as quick fix, otherwise as refactoring
 addresses?: Diagnostic[]
}

jrieken · 2017-10-17T16:24:35Z

Some inspiration from roslyn: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/CodeFixes/CodeFix.cs,7161867d561915e6

There seems to be a QuickFix type which is more or less a code action with a reference to the diagnostic it fixes.

jrieken · 2017-10-17T16:26:06Z

/cc @DustinCampbell for some background how roslyn handles refactorings vs fixes.

DustinCampbell · 2017-10-17T18:05:25Z

In Roslyn...

CodeAction is a named edit that can be invoked by the user. It contains a series of CodeActionOperations that can represent a text edit, a workspace edit, or something else. Often that "something else" is a dialog that allows the user to configure the CodeAction before invoking it. A great example of a CodeAction that requires a dialog is something like Change Signature.
A CodeFix encapsulates a set of CodeAction objects with a Diagnostic.
A CodeRefactoring encapsulates a set of CodeAction objects with a span in the editor, which could be collapsed as a caret position.

Does that help?

mjbvz · 2017-10-18T07:29:49Z

Thanks @DustinCampbell! Very helpful design to review. Do you happen to know how VS sets up keybindings for specific refactoring, such as ctrl+r, m for extract method? Is this done per-language or is there a general API for hooking these up?

mjbvz · 2017-10-18T07:37:05Z

@jrieken Today @kieferrm and I talked through the design again and cut it back to focus on address three problems:

No way to associate a quick fix with the diagnostic it addresses
No way to preview a quick fix's change
Refactoring actions are always calculated and shown with the lightbulb when just moving around in a file

The first two are addressed by adding a new CodeAction class.

For the third, one thought is to keep refactoring as code actions for now, but give code action providers additional context so that they can decide which actions to return. This latest change enables this by adding a quickFixesOnly flag on the CodeActionContext. When this flag is set, the refactoring provider would not return any code actions. The flag would always be set when just moving the cursor through code but unset if you manually request code actions with cmd+..

DustinCampbell · 2017-10-18T11:32:52Z

Do you happen to know how VS sets up keybindings for specific refactoring, such as ctrl+r, m for extract method? Is this done per-language or is there a general API for hooking these up?

There's no general API for this. It's done on a case-by-case basis. 😞

jrieken · 2017-11-08T08:42:22Z

src/vs/editor/contrib/quickFix/browser/quickFixCommands.ts

@@ -39,13 +46,15 @@ export class QuickFixController implements IEditorContribution {
 	constructor(editor: ICodeEditor,
 		@IMarkerService markerService: IMarkerService,
 		@IContextKeyService contextKeyService: IContextKeyService,
-		@ICommandService commandService: ICommandService,
+		@ICommandService private readonly commandService: ICommandService,


_ for privates please

jrieken · 2017-11-08T08:48:37Z

src/vs/editor/common/modes.ts

@@ -284,7 +292,7 @@ export interface CodeActionProvider {
 	/**
 	 * Provide commands for the given document and range.
 	 */
-	provideCodeActions(model: editorCommon.IReadOnlyModel, range: Range, token: CancellationToken): Command[] | Thenable<Command[]>;
+	provideCodeActions(model: editorCommon.IReadOnlyModel, range: Range, token: CancellationToken): (Command | CodeAction)[] | Thenable<(Command | CodeAction)[]>;


In this land we can break the API so I'd suggest we don't carry along the Command | CodeAction notion but just make it a CodeAction. I'd also do that on the protocol layer and do the Command becomes CodeAction thing in the code action adapter.

I'm concerned this will break the vscode.executeCodeActionProvider command. I believe this should still return commands instead of code actions for old providers and so I'm going to revert the change to only use code actions for now

jrieken · 2017-11-08T08:48:58Z

src/vs/editor/contrib/quickFix/browser/quickFix.ts


-	const allResults: Command[] = [];
+	const allResults: (Command | CodeAction)[] = [];


CodeAction ftw

jrieken · 2017-11-08T08:49:30Z

src/vs/editor/contrib/quickFix/browser/quickFixModel.ts

+					if ('id' in action) {
+						// must be a command
+						const command = action as Command;
+						return { title: command.title, command: command } as CodeAction;


CodeAction ftw, lets do the conversion in the ext host

jrieken · 2017-11-08T08:50:23Z

src/vs/editor/contrib/quickFix/browser/quickFixWidget.ts

+	constructor(
+		private readonly editor: ICodeEditor,
+		private readonly contextMenuService: IContextMenuService,
+		private readonly onApplyCodeAction: (action: CodeAction) => TPromise<any>


unnecessary change, _ for privates

jrieken · 2017-11-08T08:52:31Z

src/vs/workbench/api/node/extHostLanguageFeatures.ts

+				return {
+					title: codeAction.title,
+					command: codeAction.command ? this._commands.toInternal(codeAction.command) : undefined,
+					edits: codeAction.edits


For the sake of simplicity we can also make edits always a WorkspaceEdit. We'd treat the TextEdit as sugar and translate it into a workspace edit... Makes the internal CodeAction simpler and easier to handle.

jrieken · 2017-11-08T08:58:09Z

I would have gone ahead but there are some merge-conflicts...

Adds skeleton on a new CodeActionType and allows codeActionProvider to return either `Command`s or `CodeAction`s Move proposed CodeAction API to proposed and try using it in TS Split CodeAction into quickfix and refactoring classes Update proposed interface Update for new API Adding basic docs

…nd` in modes since this will break `vscode.executeCodeActionProvider`

mjbvz requested a review from jrieken October 16, 2017 03:24

egamma assigned jrieken Oct 16, 2017

mjbvz force-pushed the code-action branch 2 times, most recently from 93ee798 to 305d6a6 Compare October 16, 2017 21:32

mjbvz changed the title ~~Add CodeAction skeleton~~ Prototyping new CodeAction API Oct 17, 2017

mjbvz force-pushed the code-action branch from 1cfa071 to 59f2e09 Compare October 18, 2017 06:51

mjbvz force-pushed the code-action branch 2 times, most recently from b710b7d to 3f5b738 Compare October 25, 2017 22:38

mjbvz force-pushed the code-action branch from 3f5b738 to b24ee60 Compare November 7, 2017 04:41

jrieken requested changes Nov 8, 2017

View reviewed changes

mjbvz force-pushed the code-action branch from b24ee60 to 077f59a Compare November 8, 2017 22:29

mjbvz added 6 commits November 8, 2017 14:32

Support workspace edits and text edits in codeactions

b52ec8c

Remove placeholders

f88dc40

Resolving conflicts and making PR suggested changes

608026c

Fix quick fix test

f1e6cdd

Revert change to only use CodeAction instead of `CodeAction | Comma…

19cae05

…nd` in modes since this will break `vscode.executeCodeActionProvider`

mjbvz force-pushed the code-action branch from 077f59a to 19cae05 Compare November 8, 2017 22:59

mjbvz added this to the November 2017 milestone Nov 9, 2017

mjbvz merged commit be88547 into microsoft:master Nov 9, 2017

github-actions bot locked and limited conversation to collaborators Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prototyping new CodeAction API #36316

Prototyping new CodeAction API #36316

mjbvz commented Oct 16, 2017

mjbvz commented Oct 17, 2017

jrieken commented Oct 17, 2017

jrieken commented Oct 17, 2017 •

edited

Loading

jrieken commented Oct 17, 2017

DustinCampbell commented Oct 17, 2017

mjbvz commented Oct 18, 2017

mjbvz commented Oct 18, 2017

DustinCampbell commented Oct 18, 2017 •

edited

Loading

jrieken Nov 8, 2017

jrieken Nov 8, 2017

mjbvz Nov 8, 2017

jrieken Nov 8, 2017

jrieken Nov 8, 2017

jrieken Nov 8, 2017

jrieken Nov 8, 2017

jrieken commented Nov 8, 2017


		const allResults: Command[] = [];
		const allResults: (Command \| CodeAction)[] = [];

Prototyping new CodeAction API #36316

Prototyping new CodeAction API #36316

Conversation

mjbvz commented Oct 16, 2017

mjbvz commented Oct 17, 2017

jrieken commented Oct 17, 2017

jrieken commented Oct 17, 2017 • edited Loading

jrieken commented Oct 17, 2017

DustinCampbell commented Oct 17, 2017

mjbvz commented Oct 18, 2017

mjbvz commented Oct 18, 2017

DustinCampbell commented Oct 18, 2017 • edited Loading

jrieken Nov 8, 2017

Choose a reason for hiding this comment

jrieken Nov 8, 2017

Choose a reason for hiding this comment

mjbvz Nov 8, 2017

Choose a reason for hiding this comment

jrieken Nov 8, 2017

Choose a reason for hiding this comment

jrieken Nov 8, 2017

Choose a reason for hiding this comment

jrieken Nov 8, 2017

Choose a reason for hiding this comment

jrieken Nov 8, 2017

Choose a reason for hiding this comment

jrieken commented Nov 8, 2017

jrieken commented Oct 17, 2017 •

edited

Loading

DustinCampbell commented Oct 18, 2017 •

edited

Loading